- 
                Notifications
    
You must be signed in to change notification settings  - Fork 16
 
          ENH: apply_where (migrate lazywhere from scipy)
          #141
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| if is_jax_namespace(xp): | ||
| # jax.jit does not support assignment by boolean mask | ||
| return xp.where(cond, f1(*args), f2(*args) if f2 is not None else fill_value) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JAX-on-dask is currently unsupported by Dask. This is here and not much higher above only for this reason.
We'll need to add explicit unit tests when it lands in the future.
| ncond = ~cond | ||
| temp2 = f2(*(arr[ncond] for arr in args)) | ||
| out = xp.empty(cond.shape, dtype=dtype, device=device) | ||
| out = at(out, ncond).set(temp2) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JAX doesn't benefit from this at, but Sparse will (eventually)
41e26ad    to
    f8a633e      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @crusaderky. Overall looks good, a few comments. Didn't test locally yet, will do once this is no longer Draft.
2d199dd    to
    0b7fce1      
    Compare
  
    apply_where (migrate lazywhere from scipy)apply_where (migrate lazywhere from scipy)
      apply_where (migrate lazywhere from scipy)apply_where (migrate lazywhere from scipy)
      bf1e1e6    to
    b686fe3      
    Compare
  
    apply_where (migrate lazywhere from scipy)apply_where (migrate lazywhere from scipy)
      c9be347    to
    e307525      
    Compare
  
    71aebee    to
    ecc4931      
    Compare
  
    f027734    to
    fed0d81      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review (all apart from apply_where itself and its tests)
c3c6337    to
    b3808e7      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed all apart from TestApplyWhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @crusaderky, LGTM once open comments are resolved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you like to move the discussion at #141 (comment) to an issue somewhere?
| 
           @lucascolley all comments have been addressed  | 
    
* ENH: apply_where (migrate _lazywhere from scipy) * Code review * merge main * tweak sparse skip
Closes #14.
scipy._lib._utils.lazywhere, with revised UI and added support for jax.jit and Dask.